Codecov Report

Merging #20772 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #20772      +/-   ##
==========================================
+ Coverage   91.77%   91.78%   +<.01%     
==========================================
  Files         153      153              
  Lines       49313    49322       +9     
==========================================
+ Hits        45259    45268       +9     
  Misses       4054     4054

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 563a6ad...68b8b99. Read the comment docs.

I would also deprecate settig for DTI/TDI, and direct to using .asfreq. I don't like the idea of disallowing on PI but not the others.

Can implement .asfreq and deprecate the setters for DTI/TDI in the next day or so.

cc @jorisvandenbossche : Thoughts on this? Your preference in the issue was to allow setting for DTI/TDI and not implement .asfreq.

Yes, -1 on asfreq. As I tried to argue on the issue, it is something different as PeriodIndex.asfreq, so we should not call it the same. Directly setting the attribute is fine IMO if you need this.

I don't like the idea of disallowing on PI but not the others.

I think this difference is acceptable, because freq also has a different notion for both: for PI the freq is required (you cannot have a PI without a freq), and the underlying int values are dependent on this freq. So changing the freq is either buggy because the values and the freq no longer match (current behaviour), or it converts the underlying int values to match the new freq (but this is what asfreq already does, and since this alters the data, I think should not be done by setting an attribute).
On the other hand, for DTI and TDI, the freq is not required, and setting it allows you to add one. But this only changes the freq to the inferred_freq and does not alter the data (as otherwise an error is raised).

(note: I tagged for 0.23, but it does not need to hold up the rc)

(note: I tagged for 0.23, but it does not need to hold up the rc)

If this is going in 0.23, then I'd like it to be in the RC. Ideally we only merge bugfixes for the RC to the 0.23 release branch.

I more or less have code ready that addresses @jreback's comments, but am a bit hesitant to push since there isn't agreement on if the freq setter should be deprecated, and if so what the name of the alternative method would be.

This commit would go on top of the current changes in this PR to address @jreback's comments, in case anyone cares too look while things are still being discussed here: jschendel@2fca775

The CI services that are hooked to my repo all ran successfully, and I didn't see any warnings related to my changes in the logs. Didn't want to mess around with this PR too much while there's still ongoing discussion.

So I have a clear -1 on re-using the name asfreq for this, as I explained before (asfreq on Period is doing something different, I haven't see anybody giving answers to that argument).

If we want to keep the possibility of setting the freq, I don't mind it being the direct setting of the freq attribute (I don't think it is worth to introduce another method for this)

the inconcistenty with that you cannot set PI is pretty jaring.

I don't mind allowing setting on a PI. But if we follow the same rules as for setting freq on DTI/TDI (= only a freq that matches the data, so it does not change the interpretation of the data), then for PI this would mean that only the freq it already has will work, all other freqs will raise an exception.
I am fine with allowing that if you really want that for consistency, but it's not particularly useful.

So I have a clear -1 on re-using the name asfreq for this, as I explained before (asfreq on Period is doing something different, I haven't see anybody giving answers to that argument).

Can you repeat I can't find what you said.

I am fine the the PI change here. The issue comes down to whether we should allow one to change freq on the immutable DTI/TDI. I see no good reason for this. If you want to set the freq, simply create another one, or we make availble .asfreq on TDI/DTI (which I am little less excited about). I think having a method only on PI is fine. However the setting of an attribute is just plane wrong (yes we allow it for .name, but recently removed it for .tz as that was buggy).

I have said it here #20772 (comment) and here #20678 (comment). But it actually just boils down to "for PI asfreq changes the underlying data (it is for conversion of one frequency to another), for DTI/TDI setting the freq just updates an informative attribute that does not change the data at all"

However the setting of an attribute is just plane wrong

There is nothing "plain wrong" about setting an attribute, it is perfectly valid and depending on the use case often even preferable python code (I think often having a lot of "set_.." methods is seen as "old-java-style" while just having a property with getter and setter under the hood is more pythonic).

However the setting of an attribute is just plane wrong

On an immutable object this is True. We have 1 and only 1 attribute that we allow to be changed on an Index and that is the name. All others give you back a new object. Why break this paradigm?

Further I don't buy your argument. Whether .asfreq happens to do work on a PI, and doesn't on a TDI/DTI is not a compelling argument.

Is there a compelling use-case to actually set freq (whether inplace or return a new object) on a DTI/TDI?

Is there a compelling use-case to actually set freq (whether inplace or return a new object) on a DTI/TDI?

I made this PR after noticing a few SO questions that worked around the buggy setting behavior. That doesn't necessarily there's actually a valid use case for it, as there may be more direct workarounds for the issues people are having, but at the very least people are trying to use it.

I think one of the issues is that people generally default to using pd.to_datetime for parsing, which as far as I can tell does not have the ability to set a frequency on creation, and so they then want to set a frequency after creation. This might be more of an argument for a freq parameter in pd.to_datetime though. There also isn't much documentation for the freq='infer' option of the DTI constructor, so I also see people wanting to set the inferred frequency after creation, without actually knowing what that frequency is (again, more of a documentation issue).

I could also imagine a use case where one has messy data which needs to be cleaned first, prior to actually setting the freq. So a workflow like parse -> clean -> set frequency. Some cleaning operations may take care of the setting the freq, but there are cases where multiple frequencies are possible, so the correct one may not be set, or a custom frequency may be desired, or maybe no frequency is desired and a user wants to remove by setting None.

Whether .asfreq happens to do work on a PI, and doesn't on a TDI/DTI is not a compelling argument.

To add another data point, Series.asfreq and DataFrame.asfreq will also do work on an underlying DTI (doesn't appear to work for TDI).

Not particularly invested in being for/against reusing the asfreq name though, as there are downsides to both without a clear winner in my mind (potential for confusion with reuse vs. API bloat for new method).

@jschendel thanks for the color. But why are people actually setting the freq. This doesn't actually do anything (I don't think). Anytime we need the freq we actually infer it (again for DTI/TDI)

Setting the freq enables a couple operations that fail without one:

In [2]: dti = pd.DatetimeIndex(['20180101', '20180102', '20180103'])

In [3]: dti
Out[3]: DatetimeIndex(['2018-01-01', '2018-01-02', '2018-01-03'], dtype='datetime64[ns]', freq=None)

In [4]: dti.shift(1)
---------------------------------------------------------------------------
NullFrequencyError: Cannot shift with no freq

In [5]: dti + 4
---------------------------------------------------------------------------
NullFrequencyError: Cannot shift with no freq

In [6]: dti = pd.DatetimeIndex(['20180101', '20180102', '20180103'], freq='B')

In [7]: dti.shift(1)
Out[7]: DatetimeIndex(['2018-01-02', '2018-01-03', '2018-01-04'], dtype='datetime64[ns]', freq='B')

In [8]: dti + 4
Out[8]: DatetimeIndex(['2018-01-05', '2018-01-08', '2018-01-09'], dtype='datetime64[ns]', freq='B')

I've seen people use operations like this to enable some shorthand for inserting the previous/next logical value into an index, e.g. dti.insert(0, dti[0] - 1)

right, we allow an integer to stand in an represent the freq. ok I would say these are ok usecases (if prob not very well documented). but that is not sufficient reason to actually allow setting of an attribute on an immutable object (rather than just having a function give you a new one). its just bad semantics.

I would be ok with

dti.asfreq('D'), makes it pretty explict. We have methods for setting things like tz's, why not freq.

On an immutable object this is True. We have 1 and only 1 attribute that we allow to be changed on an Index and that is the name. All others give you back a new object. Why break this paradigm?

Exactly because the freq does not change the index, so the index is still immutable, except for those informative attributes like name and freq. So that in sense, freq is very similar to name for a DTI/TDI.

To add another data point, Series.asfreq and DataFrame.asfreq will also do work on an underlying DTI (doesn't appear to work for TDI).

For me that is another reason not to use asfreq, as for Series this is indeed a basic form of resample. So the name asfreq is already too much overloaded, no need to add yet another meaning.

ok, so I would be on board with deprecating .freq setting as in this PR, and adding a .set_freq() as an alternative (DTI/TDI, I don't think this makes sense for PI).

This would be much more in line with our handling of names in any event. The PI changes are fine.

Agree that .asfreq might be confusing here for DTI/TDI.

@jreback Are you fine with merging this now as is? The changes it does are already good (fixing a clear bug in the current setting functionality + deprecation for PI), and the other changes you propose in your last comment can be done separately I think (on top of this PR).

(just as way to include this in the rc)

yes ok to merge now (and let’s open an issue though)

Opened #20886 for the rest.

Thanks all.